Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of unified privileges checking for CLI commands #1505

Merged
merged 6 commits into from
May 30, 2024

Conversation

jan-kolarik
Copy link
Member

Using the existing RPM lock file functionality.

There are a hard-coded list of commands that are not allowed to run without superuser privileges. Then assumeno and downloadonly options are taken into account. Lastly, we check if provided command has defined the store option which should be defined for all transactional commands and therefore it should mean a need for elevated privileges.

For: #849.
Successor of: #1504.

Drop from `clean` command as it is not transactional and add it to `builddep`.
The functionality is needed by both the library and the dnf5 app.
A single checkpoint was implemented after parsing command-line data but before downloading metadata and processing commands, ensuring unified handling of cases where commands are executed under insufficient privileges.
@m-blaha
Copy link
Member

m-blaha commented May 28, 2024

I have several concerns regarding this pull request:

Firstly, I'm not sure that the centralized cmd_requires_privileges() function is the best choice. I'd prefer to delegate the decision of whether elevated privileges are required to the commands themselves, perhaps by adding a bool Command::privileges_required() method. For example, imagine a third-party dnf5 plugin implementing a new command that does not run a transaction but requires administrator privileges for some reason. How can this be handled with the current design? The plugin could be completely private to a customer, and dnf5 would not know anything about it, not even its name, to handle it in the centralized function.

Secondly, I don't think that having read/write access to <install root>/run/dnf/rpmtransaction.lock is sufficient to determine if a user has the necessary privileges to run the transaction. Consider the example mentioned in this comment #849 (comment). Although the user has full control over the specified install root, they still cannot run the RPM transaction successfully due to a failed chown() call.

@jan-kolarik
Copy link
Member Author

jan-kolarik commented May 28, 2024

Hi Marek,

I'd prefer to delegate the decision of whether elevated privileges are required to the commands themselves, perhaps by adding a bool Command::privileges_required() method

Yeah, I was also thinking about this approach. Just this would require changing each command and making this a prerequisite when implementing a new one. This PR is a simple approach to the things.

For example, imagine a third-party dnf5 plugin implementing a new command that does not run a transaction but requires administrator privileges for some reason

I was focused just on the core commands and plugins here. The 3rd party plugins could handle the situation in their way. Anyway, the current PR is really just trying to setup a single place where the initial check of privileges happens. If user doesn't have privileges for the operation, it would be denied anyway even without this PR.

Secondly, I don't think that having read/write access to /run/dnf/rpmtransaction.lock is sufficient to determine if a user has the necessary privileges to run the transaction

Yes, it is not sufficient for this use case. This specific scenario could be handled later e.g. by the install command itself. The idea here is to handle the majority of common use cases by giving the user a unified message. In other cases, operation would fail as it was before...

@m-blaha
Copy link
Member

m-blaha commented May 29, 2024

Thanks for clarification! Let's go with this simple approach than.

@m-blaha m-blaha added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit 1b7b9e3 May 30, 2024
17 of 19 checks passed
@m-blaha m-blaha deleted the jkolarik/superuser-privileges-checking branch May 30, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants